Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properties merging should work also inside directives #2035 #2121

Merged
merged 6 commits into from
Jul 27, 2014

Conversation

SomMeri
Copy link
Member

@SomMeri SomMeri commented Jul 25, 2014

Fixes issue #2035 - property merge inside @font-face. The _mergeRules function is now called also for directives with rules. It used to be called only for rulesets.

I had to turn off jasmine tests for merge.less, because it was replacing all urls by their assumed full paths. For example, the url(something.eot) was changed into url(http://localhost:8081/test/less/something.eot). The result did not matched with expected css and failed.

Note: I'm not sure why values order in source map changed. It does not seem to be caused by my change, it was failing before I made them.

SomMeri and others added 4 commits January 22, 2014 14:25
Fixes issue less#2035 - property merge inside @font-face. The _mergeRules function is now called also for directives with rules. It used to be called only for rulesets.

I had to turn off jasmine tests for merge.less, because it was replacing all urls by their assumed full paths. For example, the url(something.eot) was changed into url(http://localhost:8081/test/less/something.eot). The result did not matched with expected css and failed.

Note: I'm not sure why values order in source map changed. It does not seem to be caused by my change, it was failing before I made them.
…eird

message and I want to see what it will do this time.

Previous travis error:
>> PhantomJS has crashed. Please read the crash reporting guide at
>> https://github.com/ariya/phantomjs/wiki/Crash-Reporting and file a bug
>> report at https://github.com/ariya/phantomjs/issues/new with the crash
>> dump file attached: /tmp/7685d81d-94f5-1db7-12c9006e-72f80442.dmp 0 [
>> 'PhantomJS has crashed. Please read the crash reporting guide at
>> https://github.com/ariya/phantomjs/wiki/Crash-Reporting and file a bug
>> report at https://github.com/ariya/phantomjs/issues/new with the crash
>> dump file attached: /tmp/7685d81d-94f5-1db7-12c9006e-72f80442.dmp' ]
@lukeapage
Copy link
Member

I had to turn off jasmine tests for merge.less, because it was replacing all urls by their assumed full paths. For example, the url(something.eot) was changed into url(http://localhost:8081/test/less/something.eot). The result did not matched with expected css and failed.

I would put the merge test into urls which uses special options so the paths don't get altered (from memory)

Note: I'm not sure why values order in source map changed. It does not seem to be caused by my change, it was failing before I made them.

That is strange, it doesn't ring a bell - I would expect master to fail if that was the case - I can't explain it!

@SomMeri
Copy link
Member Author

SomMeri commented Jul 27, 2014

@lukeapage @seven-phases-max explained the source map problem in another pull request.

I will move the test into urls.less and re-enable merge.less. However, urls.less are disabled in jasmine tests (see !test/less/urls.less in current grunt.js.

jurcovicovam added 2 commits July 27, 2014 14:02
@SomMeri
Copy link
Member Author

SomMeri commented Jul 27, 2014

I moved tests into urls.less (the one in root) and re-enabled jasmine for merge.less.

lukeapage added a commit that referenced this pull request Jul 27, 2014
Properties merging should work also inside directives #2035
@lukeapage lukeapage merged commit 4b40773 into less:master Jul 27, 2014
@lukeapage
Copy link
Member

fantastic, good work

@SomMeri SomMeri deleted the font-face-property-merge-2-2035 branch July 27, 2014 19:59
@SomMeri SomMeri restored the font-face-property-merge-2-2035 branch July 28, 2014 11:18
@SomMeri SomMeri deleted the font-face-property-merge-2-2035 branch July 28, 2014 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants